Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: remove prettier and add internal_exports to eslintignore #4378

Merged
merged 7 commits into from
Feb 8, 2020

Conversation

kaisermann
Copy link
Member

@kaisermann kaisermann commented Feb 6, 2020

Even with the project having a .eslintrc and a .prettierrc, I noticed some spacing, quoting and other types of simple inconsistencies throughout the codebase. A repo this big, with a growing number of contributors, can benefit from having an enforced coding style guide. Implementations and PR reviews can be much quicker if everyone is formatting their code in the same way.

In this PR I added husky and lint-staged which helps guaranteeing that all parts of modified files (it works with partial commits!) are linted and formatted according to the project's config.

  • Added husky to easily access git-hooks
  • Added lint-staged to lint only what is being staged in a commit (also works with git add -p)
  • Added eslint-plugin-prettier/eslint-config-prettier to enforce the rules defined on the .prettierrc file
  • Created a .prettierignore file to ignore files that shouldn't be formatted by prettier.

I understand that this configs can lead to a LOT of changes if merged, since a bunch of files are not exactly following the linter/formatter config, but I really think it's worth the small trouble of running a eslint --fix && prettier --write in the whole project at some point.

Here's a PR example with the whole project formatted for comparison reasons: https://github.com/kaisermann/svelte/pull/1/files. I used prettier default options, which means a printWidth of 80 characters per line. Personally I find 80 characters much more readable, but the project could use 100 and be a little less aggressively formatted.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.

  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.

  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests with npm test or yarn test)

.prettierignore Outdated
@@ -0,0 +1 @@
package.json
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have to ignore the package.json, otherwise prettier will format it with tabs

@kaisermann kaisermann changed the title chore: Lint and format before committing chore: Enforce prettier/eslint configs and lint and format before committing Feb 6, 2020
@kaisermann kaisermann changed the title chore: Enforce prettier/eslint configs and lint and format before committing chore: enforce prettier/eslint configs and lint and format before committing Feb 6, 2020
@kaisermann
Copy link
Member Author

kaisermann commented Feb 6, 2020

Doing some tests with formatting the whole project and already caught a problem. There are some (+- 6) @ts-ignore comments that stop working because of prettier breaking the ignored line into multiple lines.

Example:

// @ts-ignore todo: should it be this.fragment.nodes[0].node.data instead?
b`${node}.textContent = ${string_literal(this.fragment.nodes[0].data)};`

to

// @ts-ignore todo: should it be this.fragment.nodes[0].node.data instead?
b`${node}.textContent = ${string_literal(
	this.fragment.nodes[0].data
)};`

They are pretty easy to fix. It's just a matter of moving the comment or adding a new one. Anyway, will dig a little deeper to know if these can be properly fixed instead of relying on the ignore comment.

@kaisermann kaisermann changed the title chore: enforce prettier/eslint configs and lint and format before committing chore: enforce prettier/eslint configs and lint/format before committing Feb 6, 2020
@Conduitry
Copy link
Member

Pre-commit hooks are something that have come up before in #4123, which I closed because I don't really care for them.

The ESLint and Prettier ignore files look like reasonable changes.

Trailing commas are something that I was trying to push for (and I included when writing the prettierrc), but have been mostly abandoned in new code because Rich doesn't liking them. (His argument is that you spend a lot more of your time looking at code than looking at diffs, which is true. But I suspect part of it also is that, when he's left to his own devices, the git history is fairly illegible anyway, but that's part of a different conversation. 😷)

We haven't been running Prettier on new code at all for some time now (partly because it makes a complete mess of our extensive use of template literals), and I'd probably be in favor of completely removing it, now that eslint/typescript integration is in a better place.

@kaisermann
Copy link
Member Author

kaisermann commented Feb 6, 2020

Yeah, It's really annoying having to deal with prettier formatting template literals awkwardly to prevent reaching the max line width. Removing prettier would definitely help in this case, but we could also use a bigger line max width which could maybe solve this particular issue. My biggest pain point with prettier in the project at the moment is having everything auto-formatted because, as you said, new code is not being run through prettier and the config is right there, waiting to be properly used 😆.

Anyway, I can modify the PR to focus only on the eslint part, but I still would like to standardize quote type and other small formatting issues (simple formatting/styling related rules only). Just for some cognitive sanity 🙏

About the git hook, yeah, I was thinking about all the dependencies and scripts that it adds, but thought that the advantages would outweigh them. No problem in removing it if the CI lint task is enough 👍

@Conduitry Conduitry merged commit 12a9ff2 into sveltejs:master Feb 8, 2020
@kaisermann kaisermann deleted the chore/lint-format-on-commit branch February 8, 2020 17:21
@kaisermann kaisermann changed the title chore: enforce prettier/eslint configs and lint/format before committing chore: remove prettier and add internal_exports to eslintignore Feb 8, 2020
jesseskinner pushed a commit to jesseskinner/svelte that referenced this pull request Feb 27, 2020
@Conduitry Conduitry mentioned this pull request Apr 14, 2020
taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants